Skip to content

fix: test-suite stability and drone camera pinhole#2545

Open
Danny024 wants to merge 3 commits into
dimensionalOS:mainfrom
Danny024:fix/test-stability-and-drone-pinhole
Open

fix: test-suite stability and drone camera pinhole#2545
Danny024 wants to merge 3 commits into
dimensionalOS:mainfrom
Danny024:fix/test-stability-and-drone-pinhole

Conversation

@Danny024

Copy link
Copy Markdown

Three independent fixes surfaced while running the test suite and the demo simulation. Each is its own commit.

1. fix(core) — flaky test_basic_deployment

The test asserted >= 8 messages after a fixed time.sleep(1). Under heavy parallel load (pytest -n auto) the publisher and LCM transport threads get starved and deliver fewer within that window, failing with assert 5 >= 8. It now polls up to a 15s deadline — still verifying end-to-end delivery through the deployment, without assuming a wall-clock throughput.

2. fix(tests) — resilient agent_setup teardown

When an agent test fails mid-run (e.g. the LLM is slow under load and finished_event.wait(60) times out), a single raising cleanup step skipped the remaining ones, leaking LCM transport threads. The autouse monitor_threads fixture then flagged the leak, turning one failure into a failure and a teardown error. A shared teardown_agent_setup() helper now runs every cleanup step (coordinator, transports, unsubs) best-effort and logs failures instead of propagating. Wired into both agent_setup fixtures.

3. fix(drone) — camera pinhole for the 3D view

The drone camera feed is logged at world/video, inside the 3D view's world origin, but no Pinhole was ever logged — so Rerun warned "2D visualizers require a pinhole ancestor to be shown in a 3D view" and the feed only rendered in the dedicated 2D pane. A static Pinhole is now logged at world/video, built from the camera intrinsics (pulled into a shared _CAMERA_INTRINSICS constant so they can't drift from the camera module).

Testing

  • test_basic_deployment passes under deliberate full-CPU starvation (previously failed assert 5 >= 8).
  • 3 navigation + 28 mcp tool-stream tests pass with clean teardown via the new helper.
  • drone-basic replay boots cleanly; the static pinhole logs at bridge start without error.
  • ruff check and ruff format --check clean on all changed files.

The test asserted >=8 messages after a fixed 1s sleep, but under heavy parallel load (pytest -n auto) the publisher/LCM threads get starved and deliver fewer, failing with 'assert 5 >= 8'. Poll up to a 15s deadline instead; this still verifies end-to-end delivery without assuming a wall-clock throughput.
@greptile-apps

greptile-apps Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Three targeted fixes for test stability and a Rerun visualization gap in the drone blueprint. No functional behavior changes to production code paths.

  • Test polling: test_basic_deployment now polls for up to 15 s instead of relying on a fixed 1 s sleep, eliminating CI failures under pytest -n auto CPU starvation.
  • Resilient teardown: A shared teardown_agent_setup() helper wraps each cleanup step in a try/except, so a timeout in one step can't skip the remaining transport/unsub cleanup and cause secondary thread-leak failures.
  • Drone pinhole: A static rr.Pinhole is now logged at world/video using a shared _CAMERA_INTRINSICS constant, fixing the Rerun "2D visualizers require a pinhole ancestor" warning and enabling the camera frustum to appear in the 3D view.

Confidence Score: 5/5

Safe to merge — all three fixes are tightly scoped to test infrastructure and a static visualization component, with no changes to production data paths.

Each change is small and well-contained: the poll loop is a straightforward replacement of a fixed sleep with no logic change to the assertions themselves; the teardown helper adds error isolation without altering what gets cleaned up; and the drone pinhole is a pure additive logging call using an already-correct intrinsics constant.

No files require special attention. The two conftest.py files are nearly identical — a pre-existing pattern — but the change applied to both is consistent.

Important Files Changed

Filename Overview
dimos/utils/testing/agent_teardown.py New helper module; isolates each cleanup step, logs failures via structlog, and labels transports by class name and unsubscribes by index — clean implementation.
dimos/agents/conftest.py Replaced inline teardown block with a call to the shared teardown_agent_setup helper; semantically identical happy-path behavior, more resilient on failure.
dimos/agents/mcp/conftest.py Mirror of agents/conftest.py change; same teardown replacement, no other modifications.
dimos/core/test_core.py Polling loop with a 15 s deadline replaces the fixed 1 s sleep; correct early-exit, assertions unchanged, no cleanup regression.
dimos/robot/drone/blueprints/basic/drone_basic.py Adds _static_camera_pinhole logged at world/video and consolidates intrinsics into _CAMERA_INTRINSICS to prevent drift; pinhole matrix and resolution look correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph fix1["fix 1 · test_basic_deployment"]
        A[robot.start / nav.start] --> B{poll every 100 ms}
        B -- counts >= 8 --> C[assertions pass ✓]
        B -- deadline > 15 s --> D[assertions fail with actual counts]
    end

    subgraph fix2["fix 2 · agent_setup teardown"]
        E[yield fn] --> F[teardown_agent_setup]
        F --> G[_safe coordinator.stop]
        G --> H[_safe transport.stop × N]
        H --> I[_safe unsub × N]
        G -- raises --> G2[log error, continue]
        H -- raises --> H2[log error, continue]
        I -- raises --> I2[log error, continue]
    end

    subgraph fix3["fix 3 · drone pinhole"]
        J[_CAMERA_INTRINSICS\nfx fy cx cy] --> K[_static_camera_pinhole\nrr.Pinhole at world/video]
        J --> L[DroneCameraModule.blueprint\ncamera_intrinsics]
        K --> M[3D view shows camera frustum ✓]
        L --> N[Camera module uses same intrinsics ✓]
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    subgraph fix1["fix 1 · test_basic_deployment"]
        A[robot.start / nav.start] --> B{poll every 100 ms}
        B -- counts >= 8 --> C[assertions pass ✓]
        B -- deadline > 15 s --> D[assertions fail with actual counts]
    end

    subgraph fix2["fix 2 · agent_setup teardown"]
        E[yield fn] --> F[teardown_agent_setup]
        F --> G[_safe coordinator.stop]
        G --> H[_safe transport.stop × N]
        H --> I[_safe unsub × N]
        G -- raises --> G2[log error, continue]
        H -- raises --> H2[log error, continue]
        I -- raises --> I2[log error, continue]
    end

    subgraph fix3["fix 3 · drone pinhole"]
        J[_CAMERA_INTRINSICS\nfx fy cx cy] --> K[_static_camera_pinhole\nrr.Pinhole at world/video]
        J --> L[DroneCameraModule.blueprint\ncamera_intrinsics]
        K --> M[3D view shows camera frustum ✓]
        L --> N[Camera module uses same intrinsics ✓]
    end
Loading

Reviews (2): Last reviewed commit: "fix(drone): log a camera pinhole so the ..." | Re-trigger Greptile

Comment thread dimos/utils/testing/agent_teardown.py Outdated
Comment on lines +55 to +56
for unsub in unsubs:
_safe(unsub, "unsubscribe")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Generic unsubscribe label hides which subscription failed

Every unsubscribe failure is logged with the same step="unsubscribe" label, so when a cleanup failure is reported you cannot tell whether it was the agent_transport or the finished_transport (or any others) that raised. Since the unsubs list is an ordered collection built from append calls in the fixture, a zero-cost improvement is to include the index or the callable's repr in the step name.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — applied in the latest push (folded into the teardown commit). Each unsubscribe is now labelled by index: _safe(unsub, f"unsubscribe[{i}]"), so a failing teardown step identifies which subscription raised.

Danny024 added 2 commits June 20, 2026 13:37
When an agent test fails mid-run (e.g. the LLM is slow under load and finished_event.wait(60) times out), a raising cleanup step skipped the rest, leaking LCM transport threads and turning the failure into a failure + teardown error. Add a shared teardown_agent_setup() that runs every step (coordinator, transports, unsubs) best-effort and logs rather than propagating. Each unsubscribe is labelled by index so a failing one is identifiable.
The camera image is logged at world/video, inside the 3D view's world origin, but no Pinhole was ever logged, so Rerun warned '2D visualizers require a pinhole ancestor to be shown in a 3D view' and the feed only rendered in the 2D pane. Register a static Pinhole at world/video built from the (now shared) camera intrinsics.
@Danny024 Danny024 force-pushed the fix/test-stability-and-drone-pinhole branch from 028e879 to df228ca Compare June 20, 2026 12:37
@Danny024

Copy link
Copy Markdown
Author

Hi @leshy @mustafab0 @paul-nechifor @spomichter — the ci and autofix.ci workflows on this PR are sitting in action_required (fork PR awaiting maintainer approval), so the test suite hasn't run yet. Could one of you approve the workflow run when you get a chance? The three commits are narrowly scoped (a flaky-test fix, an agent_setup teardown hardening, and an additive drone-camera Pinhole), and Greptile's one P2 note is already addressed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant